Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade osgi jdbc add tck #2066

Closed
wants to merge 3 commits into from

Conversation

laeubi
Copy link
Contributor

@laeubi laeubi commented Feb 4, 2023

@laeubi
Copy link
Contributor Author

laeubi commented Feb 4, 2023

@Jeffery-Wasty I don't really understand how to get more details about the testfailure can you give me a hint?

@Jeffery-Wasty Jeffery-Wasty self-assigned this Feb 6, 2023
@Jeffery-Wasty
Copy link
Member

Jeffery-Wasty commented Feb 6, 2023

@laeubi, we'll have to look into this. Previously the errors were caused by eclipse.osgi dependency, but now I'm not seeing that error in our tests, I'll get back to you with more information. Regardless, we'll look at the upgrade to osgi.service and the new properties and see if we can get that merged first, those two changes look fine. Thank you for your work on this.

@Jeffery-Wasty
Copy link
Member

Can you please confirm for us the necessity and use of the test? Is implementation of this test required by spec, or is this just a confirmation that the properties have been added correctly. It looks like the issues are all caused by the added test, so if it can be omitted, that would help us.

@laeubi
Copy link
Contributor Author

laeubi commented Feb 6, 2023

It is not strictly required, this just ensures that the implementation is fulfilling the spec.
So if it can even be omitted, it would still be interesting why this fails (I can't really get an idea from the checks output), and I was not able to run the tests locally.

@Jeffery-Wasty
Copy link
Member

Thanks for the explanation. We're still looking into it, its not readily apparent from the logs why this is failing, but we'll keep you updated on progress regarding this pr.

Copy link
Member

@lilgreenbird lilgreenbird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are there multiple PRs on this? Does this obsolete #2064 and #2065 if so please close those PRs. Can you please add some details and references to why this change is necessary.

@Jeffery-Wasty
Copy link
Member

Hi @laeubi,

The issue is that the test is that its using Junit5, and when this is invoked, causes issues in other tests in the project. If the tests can be rewritten only using features of Junit4 or below, that will allow it to pass and be included as part of the changes.

Comparing the test styles from this PR and #2017, I think the one in 2017 fits the project better, as opposed to the additional test information in the pom. Let's move forward with 2017 being the only PR for this change. We'll close the others, but thank you for this layered approach, it really helped in narrowing down on the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants